Skip to content

cleanup for GSD layout#2104

Merged
marcaaron merged 9 commits into
Expensify:masterfrom
parasharrajat:parasharrajat/GSD
Apr 1, 2021
Merged

cleanup for GSD layout#2104
marcaaron merged 9 commits into
Expensify:masterfrom
parasharrajat:parasharrajat/GSD

Conversation

@parasharrajat

@parasharrajat parasharrajat commented Mar 26, 2021

Copy link
Copy Markdown
Member

@marcaaron Please review.

Fixes

fixes #2123

Details

Following up on the changes mentioned #2015 (review)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Internal Code changes thus no screenshots.

@parasharrajat parasharrajat requested a review from a team as a code owner March 26, 2021 12:10
@botify botify requested review from joelbettner and removed request for a team March 26, 2021 12:10
Comment thread src/pages/home/sidebar/OptionRow.js Outdated
@parasharrajat parasharrajat requested a review from marcaaron March 26, 2021 21:01
@parasharrajat

Copy link
Copy Markdown
Member Author

Updated. Thanks.

@marcaaron marcaaron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes looks much better. Have one more thought and then I'll stop bugging you 😅

Comment thread src/pages/home/sidebar/OptionRow.js Outdated
@shawnborton

Copy link
Copy Markdown
Contributor

@parasharrajat see my comment over here: #2123 (comment)

In case we want to fix that while you are addressing clean up items here.

@parasharrajat

Copy link
Copy Markdown
Member Author

@shawnborton Thanks for mapping that issue to the PR. It's been taken care of.

@parasharrajat parasharrajat requested a review from marcaaron March 30, 2021 18:04
Comment thread src/styles/styles.js
},

focusedAvatar: {
backgroundColor: themeColors.pillBG,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These variables were removed recently so I have to migrate.

Comment thread src/pages/home/sidebar/OptionRow.js
Comment thread src/pages/home/sidebar/OptionRow.js
Comment thread src/pages/home/sidebar/OptionRow.js
@marcaaron

Copy link
Copy Markdown
Contributor

@parasharrajat you've got some conflicts here now and please try to remove all inline styles. We should not be passing any objects directly to a style prop. If you need to return a style object based on some props create a helper method and add it to styles.js like we have done here.

https://github.com/Expensify/Expensify.cash/blob/0867cbf5882e416766e120144a9fcee20b219682/src/styles/styles.js#L1370-L1405

@parasharrajat

Copy link
Copy Markdown
Member Author

@marcaaron Updated. Thanks.

@marcaaron marcaaron merged commit 2762798 into Expensify:master Apr 1, 2021
@OSBotify

OSBotify commented Apr 1, 2021

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@roryabraham

roryabraham commented Apr 2, 2021

Copy link
Copy Markdown
Contributor

I believe this PR may have caused a regression where the background between the avatars becomes black when the option is selected:

Not Selected Selected
image image

Note: reproducible on staging but not production so this should be a deploy blocker.

@parasharrajat

Copy link
Copy Markdown
Member Author

Oh. I think this PR should fix this. Let me check.

@roryabraham

Copy link
Copy Markdown
Contributor

Oops, ignore me, I was not on the latest staging version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Group creation - Not enough space between circle and edge

5 participants